Added Versat, crypto software implementations and versat implementations as well. Added tests and they all pass #82

zettasticks · 2024-02-26T14:15:19Z

There is some warnings when simulating from versat on icarus but nothing serious.

Tests for SHA, AES and McEliece all pass. They only run during fpga-run since they take too long to sim.

default.nix had to change from a simple link to a real file in order to add the "(callPackage ./submodules/VERSAT/versat.nix { })" line.

… peripheral

…riginal repo

…g is working properly

…since they take too long and they run on fpga fine

jjts · 2024-02-27T00:20:25Z

Is this failing because the MEM_NO_READ_ON_WRITE macro is not being read?

P-Miranda

I added some comments with recomendations/sugestions
cyclonev synthesis fails fit step

P-Miranda · 2024-02-27T11:48:30Z

software/sw_build.mk

+
+#Lines below were auto generated by iob_soc_utils.py
+UTARGETS+=iob_eth_rmac.h
+EMUL_HDR+=iob_eth_rmac.h
+iob_eth_rmac.h:
+	echo "#define ETH_RMAC_ADDR 0x$(RMAC_ADDR)" > $@
+
+CONSOLE_CMD ?=rm -f soc2cnsl cnsl2soc; $(IOB_CONSOLE_PYTHON_ENV) $(PYTHON_DIR)/console_ethernet.py -L -c $(PYTHON_DIR)/console.py -m "$(RMAC_ADDR)"
+


as stated in the comment:
these lines are added automatically by iob_soc_utils.py script during setup if
iob_soc_opencryptolinux.conf has USE_ETHERNET

so we can discard this addition

Suggested change

#Lines below were auto generated by iob_soc_utils.py

UTARGETS+=iob_eth_rmac.h

EMUL_HDR+=iob_eth_rmac.h

iob_eth_rmac.h:

echo "#define ETH_RMAC_ADDR 0x$(RMAC_ADDR)" > $@

CONSOLE_CMD ?=rm -f soc2cnsl cnsl2soc; $(IOB_CONSOLE_PYTHON_ENV) $(PYTHON_DIR)/console_ethernet.py -L -c $(PYTHON_DIR)/console.py -m "$(RMAC_ADDR)"

P-Miranda · 2024-02-27T12:01:43Z

iob_soc_opencryptolinux.py

+VERSAT_SPEC = "versatSpec.txt"
+VERSAT_EXTRA_UNITS = os.path.realpath(
+    os.path.join(os.path.dirname(__file__), "hardware/src/units")
+)


why set global variables that are only used once?

Suggested change

VERSAT_SPEC = "versatSpec.txt"

VERSAT_EXTRA_UNITS = os.path.realpath(

os.path.join(os.path.dirname(__file__), "hardware/src/units")

)

P-Miranda · 2024-02-27T12:02:54Z

iob_soc_opencryptolinux.py

@@ -86,6 +98,11 @@ def _create_instances(cls):
    @classmethod
    def _create_submodules_list(cls, extra_submodules=[]):
        """Create submodules list with dependencies of this module"""
+


set variables here:

Suggested change

VERSAT_SPEC = "versatSpec.txt"

VERSAT_EXTRA_UNITS = os.path.realpath(

os.path.join(os.path.dirname(__file__), "hardware/src/units")

)

P-Miranda · 2024-02-27T12:05:57Z

iob_soc_opencryptolinux.py

+        # OpenCrypts testcases
+        shutil.copytree(
+            f"{cls.setup_dir}/tests", f"{cls.build_dir}/tests", dirs_exist_ok=True
+        )


should we just place tests under ./software/src/tests and they are automatically copied to BUILD_DIR?

or add to ./software/versat/tests and add versatSpec.txt to ./software/versat/versatSpec.txt for versat-specific files

P-Miranda · 2024-02-27T12:14:03Z

software/sw_build.mk

+# NOTE(Ruben): To speed up simulation, the following files can be commented out to greatly reduce filesize from around ~120Kb to ~20Kb.
+IOB_SOC_OPENCRYPTOLINUX_FW_SRC+=src/versat_crypto.c
+IOB_SOC_OPENCRYPTOLINUX_FW_SRC+=src/versat_crypto_tests.c
+IOB_SOC_OPENCRYPTOLINUX_FW_SRC+=src/crypto/aes.c
+IOB_SOC_OPENCRYPTOLINUX_FW_SRC+=$(wildcard src/crypto/McEliece/*.c)
+IOB_SOC_OPENCRYPTOLINUX_FW_SRC+=$(wildcard src/crypto/McEliece/common/*.c)


sugestion: add only to sw_build.mk if we are using versat? Use a setup like USE_ETHERNET?
or skip sources for simulation flows

Did everything except this part. How do I approach this in the correct python setup approach? I do not see any variable inside the Makefile that indicates we are simulating vs running fpga. Also do not see any direct way of telling on the python side, so cannot conditionally insert the lines in sw_build because cannot know if doing setup to sim or doing setup to run fpga.

An ideal solution would be to remove unused code from compilation.
After some search, it seems that adding the compilation and linking flags: -fdata-sections -ffunction-sections -Wl,--gc-sections would remove unused functions.
But it yields an elf without sections:

riscv64-unknown-elf-objcopy: error: the input file 'iob_soc_opencryptolinux_boot.elf' has no sections

I think we should just keep this as is.

sugestion: add only to sw_build.mk if we are using versat? Use a setup like USE_ETHERNET? or skip sources for simulation flows

I've added a way to skip sources for simulation in a suggestion below, but I have not tried to run it, so it may break compilation if the firmware requires those functions to be present.

P-Miranda · 2024-02-28T12:47:08Z

submodules/VERSAT

Note: new commit not present in upstream repository.
PR iob-versat changes to iobundle/iob-versat

…r cyclonev

zettasticks · 2024-02-28T13:51:36Z

Checks are failing because the timeout for cyclonev was too low, with Versat compilation takes more time and CycloneV was timing out in the middle of running Linux. Baremetal check was working in previous commit.

arturum1

Changes seem good. I have not reviewed crypto-related software code.

However, I've added a suggestion for a better implementation of the default.nix file.

I also included some suggestions about preventing inclusion of sources in the sw_build.mk file during simulation.

default.nix

software/sw_build.mk

arturum1 · 2024-02-29T01:07:56Z

software/sw_build.mk

@@ -63,14 +64,23 @@ TEMPLATE_LDS=src/[email protected]

 IOB_SOC_OPENCRYPTOLINUX_CFLAGS ?=-Os -nostdlib -march=rv32imac -mabi=ilp32 --specs=nano.specs -Wcast-align=strict

-IOB_SOC_OPENCRYPTOLINUX_INCLUDES=-I. -Isrc 
+IOB_SOC_OPENCRYPTOLINUX_INCLUDES=-I. -Isrc -Isrc/crypto/McEliece -Isrc/crypto/McEliece/common


We can include some sources only if running on the FPGA:

Suggested change

IOB_SOC_OPENCRYPTOLINUX_INCLUDES=-I. -Isrc -Isrc/crypto/McEliece -Isrc/crypto/McEliece/common

IOB_SOC_OPENCRYPTOLINUX_INCLUDES=-I. -Isrc

ifneq ($(SIMULATION),1)

IOB_SOC_OPENCRYPTOLINUX_INCLUDES+=-Isrc/crypto/McEliece -Isrc/crypto/McEliece/common

endif

I did not do this because the includes in gcc is for header lookups and stuff. None of which actually adds code so it's not a problem if they get added regardless.

software/sw_build.mk

zettasticks added 18 commits December 20, 2023 23:07

Added Versat, working except contains a bug with first read by CPU of…

374582f

… peripheral

AES256 and SHA working

368862b

Merge branch 'master' of github.com:zettasticks/iob-soc-opencryptolinux

aed40c2

Merge branch 'master' of github.com:IOBundle/iob-soc-opencryptolinux

daac59b

Updated versat

7e1ef86

Added versat, remade linux with versat, updated submodules to match o…

1a46719

…riginal repo

Working in sim-run, only missing testing on FPGA

31589d6

Added initial implementation of McEliece

e721be4

Save current impl, only giving an error around loop 570

132e7e8

Found bug, was cache problem, not good fix but saving what I have

8605d90

Tests working, still need to do a reset + test to make sure everythin…

06dff57

…g is working properly

Updated versat

8de697c

All crypto tests passing and working on fpga

1942780

Merge remote-tracking branch 'other/master'

a68ce5b

Reverted some files to match upstream, removed versat tests from sim …

9e0ecc1

…since they take too long and they run on fpga fine

Removed temporary testbench

abfc23b

Update submodules to fix errors given by ghactions

0d66b79

Update versat to fix error in tools

00ed586

jjts requested review from arturum1 and P-Miranda February 27, 2024 00:19

P-Miranda reviewed Feb 27, 2024

View reviewed changes

zettasticks added 3 commits February 27, 2024 22:45

Updated Versat, should fix error on CycloneV

1a7ef25

Removed lines that where not supposed to get inserted in .mk file

c45d031

Simple update on tests to make results easier to check

0b5f69d

P-Miranda reviewed Feb 28, 2024

View reviewed changes

submodules/VERSAT Outdated

Copy link

Contributor

P-Miranda Feb 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: new commit not present in upstream repository.
PR iob-versat changes to iobundle/iob-versat

Adding versat increased compile time so we need to bump up timeout fo…

54cb5be

…r cyclonev

Added some extra explanation in tests, commit to run checks again

bb6604b

arturum1 suggested changes Feb 29, 2024

View reviewed changes

Remove crypto side from sim, default.nix uses common LIB .nix

fbe302a

arturum1 approved these changes Feb 29, 2024

View reviewed changes

P-Miranda approved these changes Mar 4, 2024

View reviewed changes

jjts merged commit 4bbab72 into IObundle:master Mar 4, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Versat, crypto software implementations and versat implementations as well. Added tests and they all pass #82

Added Versat, crypto software implementations and versat implementations as well. Added tests and they all pass #82

zettasticks commented Feb 26, 2024

jjts commented Feb 27, 2024

P-Miranda left a comment •

edited

Loading

P-Miranda Feb 27, 2024

P-Miranda Feb 27, 2024

P-Miranda Feb 27, 2024

P-Miranda Feb 27, 2024

P-Miranda Feb 27, 2024 •

edited

Loading

zettasticks Feb 28, 2024

P-Miranda Feb 28, 2024

arturum1 Feb 29, 2024

P-Miranda Feb 28, 2024

zettasticks commented Feb 28, 2024

arturum1 left a comment

arturum1 Feb 29, 2024

zettasticks Feb 29, 2024

Added Versat, crypto software implementations and versat implementations as well. Added tests and they all pass #82

Added Versat, crypto software implementations and versat implementations as well. Added tests and they all pass #82

Conversation

zettasticks commented Feb 26, 2024

jjts commented Feb 27, 2024

P-Miranda left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

P-Miranda Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zettasticks commented Feb 28, 2024

arturum1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

P-Miranda left a comment •

edited

Loading

P-Miranda Feb 27, 2024 •

edited

Loading